Data-bound chart components 1/8: design record and ground pins - #461
Data-bound chart components 1/8: design record and ground pins#461FarhanAliRaza wants to merge 1 commit into
Conversation
The design space and decision for the reflex_xy component API revision (Option 6: data-bound chart components), its file-level work plan, and the Phase 0 tests everything above this rests on. The pins are the point: test_framework_contracts.py fixes the Reflex behavior the design depends on (parametrized-generic var types survive as _var_type, typed props reject wrong vars at create(), unknown non-event kwargs are silently absorbed into style), and test_validation_timing.py fixes xy's own validation timing (zero-row construction compiles, mark config validates at .figure(), chrome validates eagerly, Chart.figure() memoizes). Each test names the design fact it pins, so a Reflex or grammar upgrade that moves the ground fails here first rather than in the tier built on top of it. No runtime change.
📝 WalkthroughWalkthroughThis PR adds two design documents for the Reflex data-bound chart component API and adds tests that pin Reflex type-checking, component kwarg validation, zero-row chart validation, and figure rebinding behavior. ChangesReflex data-bound chart API
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds the Option 6 design record and Phase 0 contract tests for Reflex typing/kwarg behavior and xy validation timing.
Confidence Score: 4/5The PR appears safe to merge after correcting the design documents so they do not present later stack entries as already shipped. The executable changes are contract tests with no identified behavioral defect, while the authoritative implementation plan currently directs readers to APIs and files that are absent from this revision. Files Needing Attention: spec/design/reflex-component-api-implementation.md, spec/design/reflex-component-api-options.md
|
| Filename | Overview |
|---|---|
| spec/design/reflex-component-api-implementation.md | Adds the phased implementation plan, but inaccurately marks later, currently absent phases as completed. |
| spec/design/reflex-component-api-options.md | Records the design alternatives and Option 6 decision, with its top-level implemented status similarly ahead of this stack entry. |
| tests/reflex_adapter/test_framework_contracts.py | Adds focused Reflex contract pins and correctly relies on the adapter directory's optional-dependency collection guards. |
| tests/test_validation_timing.py | Adds focused tests for zero-row compilation, validation timing, and deliberate Figure memoization behavior. |
Reviews (1): Last reviewed commit: "spec+test: design record and ground pins..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/design/reflex-component-api-options.md`:
- Around line 3-11: The design documents overstate the shipped implementation
status while runtime support remains unimplemented. In
spec/design/reflex-component-api-options.md lines 3-11, change the status to
describe the selected API as planned; in
spec/design/reflex-component-api-implementation.md lines 3-13, state that only
Phase 0 landed in this PR; and in lines 329-376, remove or defer the completion
record until the runtime implementation PRs land.
In `@tests/reflex_adapter/test_framework_contracts.py`:
- Around line 15-16: Update the module-level imports in
test_framework_contracts.py so pytest is imported first, then call
pytest.importorskip("reflex") before importing reflex as rx. Preserve the
existing test behavior while allowing collection to skip cleanly when the reflex
dependency is unavailable.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 48b06c37-05e2-4ea4-ad91-041d6632716b
📒 Files selected for processing (4)
spec/design/reflex-component-api-implementation.mdspec/design/reflex-component-api-options.mdtests/reflex_adapter/test_framework_contracts.pytests/test_validation_timing.py
Merging this PR will not alter performance
Comparing 
Footnotes
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
FarhanAliRaza
changed the title
|
On the completion-record finding: rather than moving or downgrading the record, the stack now completes the implementation the record describes — the synthetic-probe design it documented was replaced by the core structural-probe seam (8/8), callables are content-addressed, worker plan distribution fails closed (5/8), the plan index is bounded by mounts unconditionally (4/8), /kinds carries browser render coverage, and the plan-tier costs are benchmarked and recorded. The record and decision docs were revised in the same commits as the code they describe (repo rule), so by the top of the stack every claim in this document is implemented and pinned. Titles are renumbered 1/8–8/8. |
masenf
left a comment
masenf
left a comment
There was a problem hiding this comment.
plan looks good. i like option 6 the best too
2 participants
First of an 8-PR stack implementing the
reflex_xydata-bound component API (Option 6). Review in order — each PR is green on its own, and checking out the last one gives you the whole thing.stack/1-design-and-pinsstack/2-typed-handlesFigureHandle, typedfigurepropstack/3-plans-and-data-varsplan.py,@reflex_xy.data, token grammarstack/4-composite-servingxyp1composites over the data planestack/5-chart-factoriesscatter_chart(data=…)and friendsstack/6-compile-probeprobe=for the escape hatchstack/7-demos-and-gallerystack/8-full-kind-coverageThis PR
The design space and decision, its file-level work plan, and the tests everything above rests on. No runtime change.
The pins are the point:
test_framework_contracts.pyfixes the Reflex behavior the design depends on — parametrized-generic var types survive as_var_type(the schema channel), typed props reject wrong vars atcreate(), and unknown non-event kwargs are silently absorbed intostyle(the hazard PR5's kwarg partition has to compensate for).test_validation_timing.pyfixes xy's own validation timing — zero-row construction compiles, mark config validates at.figure()rather than construction, chrome validates eagerly,Chart.figure()memoizes.Each test names the design fact it pins (R1/R7/R8/X1–X3), so a Reflex or grammar upgrade that moves the ground fails here first instead of in the tier built on top.
Note for review
The two design documents are ~1300 of the 1491 lines. If we'd rather not carry them in-tree, say so and I'll drop them — the ~200 lines of pins are the part that has to land.
Test plan
uv run pytest tests/reflex_adapter tests/test_validation_timing.py— 165 passedpre-commit run --all-files,ruff check,ruff format --check,ty check— clean<img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg">Summary by CodeRabbit
Documentation
Tests